feat(settings): migrate settings from modal to route-based pages#3413
feat(settings): migrate settings from modal to route-based pages#3413waleedlatif1 merged 1 commit intofeat/mothership-copilotfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Multiple settings components are refit for the route-based layout (imports/paths updated, modal-specific Navigation/UX adjustments: workspace root and invitation accept redirects now go to Written by Cursor Bugbot for commit 15b28e1. Configure here. |
apps/sim/app/workspace/[workspaceId]/files/components/files.tsx
Outdated
Show resolved
Hide resolved
0103743 to
ed5d416
Compare
|
@greptile |
|
@cursor review |
apps/sim/app/workspace/[workspaceId]/settings/components/credentials/credentials-manager.tsx
Show resolved
Hide resolved
apps/sim/app/workspace/[workspaceId]/settings/components/credentials/credentials-manager.tsx
Outdated
Show resolved
Hide resolved
ed5d416 to
b972e9e
Compare
Greptile SummaryThis PR migrates the settings experience from a modal dialog to dedicated route-based pages at Key changes and observations:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User clicks Settings] --> B[Router navigates to\n/workspace/workspaceId/settings/general]
B --> C[workspaceId layout.tsx\nrenders Sidebar + children]
C --> D{isOnSettingsPage?}
D -- Yes --> E[Render SettingsSidebar\n in sidebar slot]
D -- No --> F[Render normal\nWorkflow Sidebar]
E --> G[settings/layout.tsx\nmax-w-900px scrollable]
G --> H[settings/section/page.tsx\nServer: auth + workspace check]
H --> I{Valid session\n+ membership?}
I -- No --> J[redirect '/']
I -- Yes --> K[SettingsPage client component\nreads section from props\nreads mcpServerId from useSearchParams]
K --> L{effectiveSection?}
L -- general --> M[General]
L -- credentials --> N[Credentials]
L -- mcp --> O[MCP]
L -- subscription --> P[Subscription]
L -- ... --> Q[Other sections]
L -- unknown --> R[⚠️ Blank page\nno fallback]
E --> S[Back button\nhard-codes /home]
Last reviewed commit: 15b28e1 |
apps/sim/app/workspace/[workspaceId]/settings/[section]/settings-page.tsx
Show resolved
Hide resolved
...orkspace/[workspaceId]/w/components/sidebar/components/settings-sidebar/settings-sidebar.tsx
Show resolved
Hide resolved
b972e9e to
acff8fc
Compare
|
@greptile |
|
@cursor review |
apps/sim/app/workspace/[workspaceId]/settings/components/credentials/credentials-manager.tsx
Show resolved
Hide resolved
acff8fc to
15b28e1
Compare
|
@cursor review |
| export function SettingsPage({ section }: SettingsPageProps) { | ||
| const searchParams = useSearchParams() | ||
| const mcpServerId = searchParams.get('mcpServerId') | ||
|
|
||
| const effectiveSection = | ||
| !isBillingEnabled && (section === 'subscription' || section === 'team') ? 'general' : section | ||
|
|
||
| const label = | ||
| allNavigationItems.find((item) => item.id === effectiveSection)?.label ?? effectiveSection | ||
|
|
||
| return ( | ||
| <div> | ||
| <h2 className='mb-[28px] font-medium text-[22px] text-[var(--text-primary)]'>{label}</h2> | ||
| {effectiveSection === 'general' && <General />} | ||
| {effectiveSection === 'credentials' && <Credentials />} | ||
| {effectiveSection === 'template-profile' && <TemplateProfile />} | ||
| {effectiveSection === 'credential-sets' && <CredentialSets />} | ||
| {effectiveSection === 'access-control' && <AccessControl />} | ||
| {effectiveSection === 'apikeys' && <ApiKeys />} | ||
| {isBillingEnabled && effectiveSection === 'subscription' && <Subscription />} | ||
| {isBillingEnabled && effectiveSection === 'team' && <TeamManagement />} | ||
| {effectiveSection === 'sso' && <SSO />} | ||
| {effectiveSection === 'byok' && <BYOK />} | ||
| {effectiveSection === 'copilot' && <Copilot />} | ||
| {effectiveSection === 'mcp' && <MCP initialServerId={mcpServerId} />} | ||
| {effectiveSection === 'custom-tools' && <CustomTools />} | ||
| {effectiveSection === 'skills' && <Skills />} | ||
| {effectiveSection === 'workflow-mcp-servers' && <WorkflowMcpServers />} | ||
| {effectiveSection === 'debug' && <Debug />} | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
Invalid section parameter renders blank settings page
When a user navigates to an unrecognized section (e.g. /workspace/[id]/settings/nonexistent), effectiveSection will be a string that matches none of the conditional renders below the <h2>. The result is a page that shows only the section string as a heading with no content — no 404 or redirect to general.
The section parameter should be validated server-side in page.tsx before passing it to this component, or a fallback should be added here:
| export function SettingsPage({ section }: SettingsPageProps) { | |
| const searchParams = useSearchParams() | |
| const mcpServerId = searchParams.get('mcpServerId') | |
| const effectiveSection = | |
| !isBillingEnabled && (section === 'subscription' || section === 'team') ? 'general' : section | |
| const label = | |
| allNavigationItems.find((item) => item.id === effectiveSection)?.label ?? effectiveSection | |
| return ( | |
| <div> | |
| <h2 className='mb-[28px] font-medium text-[22px] text-[var(--text-primary)]'>{label}</h2> | |
| {effectiveSection === 'general' && <General />} | |
| {effectiveSection === 'credentials' && <Credentials />} | |
| {effectiveSection === 'template-profile' && <TemplateProfile />} | |
| {effectiveSection === 'credential-sets' && <CredentialSets />} | |
| {effectiveSection === 'access-control' && <AccessControl />} | |
| {effectiveSection === 'apikeys' && <ApiKeys />} | |
| {isBillingEnabled && effectiveSection === 'subscription' && <Subscription />} | |
| {isBillingEnabled && effectiveSection === 'team' && <TeamManagement />} | |
| {effectiveSection === 'sso' && <SSO />} | |
| {effectiveSection === 'byok' && <BYOK />} | |
| {effectiveSection === 'copilot' && <Copilot />} | |
| {effectiveSection === 'mcp' && <MCP initialServerId={mcpServerId} />} | |
| {effectiveSection === 'custom-tools' && <CustomTools />} | |
| {effectiveSection === 'skills' && <Skills />} | |
| {effectiveSection === 'workflow-mcp-servers' && <WorkflowMcpServers />} | |
| {effectiveSection === 'debug' && <Debug />} | |
| </div> | |
| ) | |
| const validSections = allNavigationItems.map((item) => item.id) | |
| const effectiveSection: SettingsSection = (() => { | |
| if (!isBillingEnabled && (section === 'subscription' || section === 'team')) { | |
| return 'general' | |
| } | |
| if (!validSections.includes(section as SettingsSection)) { | |
| return 'general' | |
| } | |
| return section | |
| })() |
| 'use client' | ||
|
|
||
| import { useSearchParams } from 'next/navigation' | ||
| import { | ||
| ApiKeys, | ||
| BYOK, | ||
| Copilot, | ||
| CredentialSets, | ||
| Credentials, | ||
| CustomTools, | ||
| Debug, | ||
| General, | ||
| MCP, | ||
| Skills, | ||
| Subscription, | ||
| TeamManagement, | ||
| TemplateProfile, | ||
| WorkflowMcpServers, | ||
| } from '@/app/workspace/[workspaceId]/settings/components' | ||
| import type { SettingsSection } from '@/app/workspace/[workspaceId]/settings/navigation' | ||
| import { | ||
| allNavigationItems, | ||
| isBillingEnabled, | ||
| } from '@/app/workspace/[workspaceId]/settings/navigation' | ||
| import { AccessControl } from '@/ee/access-control/components/access-control' | ||
| import { SSO } from '@/ee/sso/components/sso-settings' | ||
|
|
||
| interface SettingsPageProps { |
There was a problem hiding this comment.
All settings components statically imported — large bundle chunk
Every settings section component (General, MCP, Credentials, TeamManagement, etc.) is statically imported at the top of this file. Since the user only ever views one section at a time, this means the entire settings surface — including very large components like mcp.tsx (~1626 lines) and credentials-manager.tsx (~1949 lines) — is bundled into a single client chunk.
Consider using next/dynamic for the heavier section components to split them into separate lazy-loaded chunks:
import dynamic from 'next/dynamic'
const MCP = dynamic(() => import('../components/mcp/mcp').then((m) => ({ default: m.MCP })))
const Credentials = dynamic(() => import('../components/credentials/credentials').then((m) => ({ default: m.Credentials })))
// ...| <div className='flex flex-1 flex-col overflow-auto bg-white px-[24px] pt-[28px] pb-[24px] dark:bg-[var(--bg)]'> | ||
| {/* Header */} | ||
| <div> |
There was a problem hiding this comment.
Hardcoded bg-white bypasses theme CSS variable
The inner container uses bg-white for light mode rather than a CSS variable (--bg or --surface-2). If the theme defines a non-white light background (e.g. in a white-label config), this element will appear inconsistent with the rest of the UI. The dark mode counterpart already uses dark:bg-[var(--bg)] correctly.
| <div className='flex flex-1 flex-col overflow-auto bg-white px-[24px] pt-[28px] pb-[24px] dark:bg-[var(--bg)]'> | |
| {/* Header */} | |
| <div> | |
| <div className='flex flex-1 flex-col overflow-auto bg-[var(--bg)] px-[24px] pt-[28px] pb-[24px]'> |
| (options?: SettingsNavigationOptions): string => { | ||
| const section = options?.section || 'general' | ||
| const searchParams = options?.mcpServerId ? `?mcpServerId=${options.mcpServerId}` : '' | ||
| return `/workspace/${workspaceId}/settings/${section}${searchParams}` | ||
| }, |
There was a problem hiding this comment.
mcpServerId appended to URL without encoding
The mcpServerId value is interpolated directly into the query string without encodeURIComponent. If a server ID ever contains characters like +, &, #, or spaces, the resulting URL will be malformed and useSearchParams().get('mcpServerId') will return an incorrect value.
| (options?: SettingsNavigationOptions): string => { | |
| const section = options?.section || 'general' | |
| const searchParams = options?.mcpServerId ? `?mcpServerId=${options.mcpServerId}` : '' | |
| return `/workspace/${workspaceId}/settings/${section}${searchParams}` | |
| }, | |
| (options?: SettingsNavigationOptions): string => { | |
| const section = options?.section || 'general' | |
| const searchParams = options?.mcpServerId | |
| ? `?mcpServerId=${encodeURIComponent(options.mcpServerId)}` | |
| : '' | |
| return `/workspace/${workspaceId}/settings/${section}${searchParams}` | |
| }, |
Summary
/workspace/[workspaceId]/settings/[section]SettingsSidebarcomponent with navigation matching workflow sidebar stylinguseSettingsNavigationhook replacing alluseSettingsModalStore+ window event dispatchersopen-settings/close-settingswindow eventscalc()settings-modal/tosettings/components/Type of Change
Testing
Tested manually
Checklist